Birmingham | 2026-MAR-SDC | Joy Opachavalit | Sprint 3 | Implement shell tools by enjoy15 · Pull Request #408 · CodeYourFuture/Module-Tools

enjoy15 · 2026-03-21T13:41:23Z

Learners, PR Template

Self checklist

I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
My changes meet the requirements of the task
I have tested my changes
My changes follow the style guide

Changelist

Completed all exercises

illicitonion

Generally looking good, but it looks like you're slightly over-fitting to the examples in the README.md - give some thought to other command lines a user may try to run, and how your implementations may be surprising to them.

illicitonion · 2026-03-27T17:04:46Z

implement-shell-tools/cat/cat.js

+  let lineNumber = 1;
+
+  files.forEach((file) => {
+    const filePath = path.resolve(file);


What is this line doing? What would break if you removed it and just used file instead of filePath?

I have removed the unnecessary use of path.resolve and now directly use file. The code works as expected without it, simplifying the implementation.

illicitonion · 2026-03-27T17:06:21Z

implement-shell-tools/cat/cat.js

+        if (options.numberNonEmpty && line.trim()) {
+          console.log(`${lineNumber}\t${line}`);
+          lineNumber++;
+        } else if (options.numberLines) {
+          console.log(`${lineNumber}\t${line}`);
+          lineNumber++;
+        } else {
+          console.log(line);


The three branches here look quite similar and repetitive. In general, if you have multiple similar branches, it's more clear to extract the differences into variables, and then run the same code, i.e. so you'd only have one call to console.log which looks more like console.log(`${prefix}${line}\n`) where prefix may be set differently based on options (including potentially an empty string).

This way it's easier for someone reading the code to see what's the same / different in each case, and also avoids the hazard that someone updates one of the branches but forgets to update the other ones.

I have refactored the code to reduce repetition. Now, the differences are handled using a prefix variable, and there is only one console.log statement. This makes the code cleaner and easier to maintain.

illicitonion · 2026-03-27T17:06:54Z

implement-shell-tools/cat/cat.js

+        }
+      });
+    } catch (err) {
+      console.error(`cat: ${file}: No such file or directory`);


What exit code will your program have if something went wrong? What exit code should it have?

I have added proper exit codes. The program now exits with process.exit(1) when an error occurs, ensuring it signals failure appropriately.

illicitonion · 2026-03-27T17:07:55Z

implement-shell-tools/cat/cat.js

+        }
+      });
+    } catch (err) {
+      console.error(`cat: ${file}: No such file or directory`);


Are you sure the problem here is that the file didn't exist? What would happen e.g. if you didn't have permission to read the file?

In general specific error messages are good, but misleadingly specific error messages are a problem - if we're not sure what went wrong (or if we have more information about what went wrong), we should present that information, rather than guessing. And if we are guessing, we should make it clear we're not sure what the problem exactly was and that this is a guess.

I have improved the error handling to differentiate between file-not-found (ENOENT) and permission errors (EACCES). The error messages now provide more accurate information about what went wrong.

illicitonion · 2026-03-27T17:16:07Z

implement-shell-tools/wc/wc.js

+    if (arg === '-l') {
+      options.lines = true;
+    } else if (arg === '-w') {
+      options.words = true;
+    } else if (arg === '-c') {
+      options.bytes = true;
+    } else {
+      files.push(arg);
+    }


What happens if someone specifies more than one of these flags? What should happen?

Given the test cases we gave you in the README file, it's ok if your implementation doesn't do the same thing as the real wc does, though that would be ideal, but in general ignoring user input is bad - so if someone asks for both -l and -c and you ignore one of them, that can be confusing. Either showing both, or giving an error, is probably preferable.

I have updated the code to handle multiple flags. If multiple flags are specified, the program now displays all the requested results together, ensuring user input is respected.

illicitonion · 2026-03-27T17:16:17Z

implement-shell-tools/wc/wc.js

+  }
+
+  files.forEach((file) => {
+    const filePath = path.resolve(file);


Again, why do you need the path.resolve here?

I have removed the unnecessary use of path.resolve and now directly use the file path provided by the user. This simplifies the code without affecting functionality.

illicitonion · 2026-03-27T17:20:22Z

implement-shell-tools/ls/ls.js

+    } else if (arg === '-a') {
+      options.all = true;
+    } else {
+      directories = [arg];


This meets the requirements for the examples listed in the README.md, but feels like it risks being confusing to users. If someone specified ls /some/path /some/other/path what do you think they would expect to happen? What does your code actually do? (See also the comment about wc options for a similar question)

I have updated the code to handle multiple directories. The program now lists files for all specified directories, making it more intuitive for users.

enjoy15 added 3 commits March 21, 2026 12:22

complete cat excercise

1576d23

complete ls exercise

34fe1d0

complete wc exercise

daac5ec

enjoy15 added 📅 Sprint 3 Assigned during Sprint 3 of this module Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 21, 2026

illicitonion reviewed Mar 27, 2026

View reviewed changes

illicitonion added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Mar 27, 2026

Refactor file handling and error messages in cat, ls, and wc tools

7e70d1c

enjoy15 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Apr 5, 2026

Uh oh!

Conversation

enjoy15 commented Mar 21, 2026

Learners, PR Template

Changelist

Uh oh!

illicitonion left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants